Skip to content

Conversation

@projjal
Copy link
Contributor

@projjal projjal commented Sep 2, 2020

Moving the castint/float functions to gdv_function_stubs outside of precompiled module

@projjal projjal changed the title ARROW-9898: Fix error handling in castINT function ARROW-9898: [C++][Gandiva] Fix error handling in castINT function Sep 2, 2020
@github-actions
Copy link

github-actions bot commented Sep 2, 2020

@kiszk
Copy link
Member

kiszk commented Sep 3, 2020

Would it be possible to add an example to cause SIGSEGV?

@projjal projjal force-pushed the castint branch 2 times, most recently from 5815a51 to 61b8485 Compare September 10, 2020 03:30
@projjal projjal changed the title ARROW-9898: [C++][Gandiva] Fix error handling in castINT function ARROW-9898: [C++][Gandiva] Fix linking issue with castINT/FLOAT functions Sep 10, 2020
@projjal projjal force-pushed the castint branch 2 times, most recently from b1b3605 to 8af48ae Compare September 15, 2020 10:01
@projjal projjal force-pushed the castint branch 2 times, most recently from 5e8059c to 36dc61d Compare October 9, 2020 04:24
@projjal
Copy link
Contributor Author

projjal commented Oct 9, 2020

Would it be possible to add an example to cause SIGSEGV?

I have added java tests that causes crashes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did we make this an external function only for this message? whats the perf penalty for doing this..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really for this message. As you can see from attached commits earlier I removed the std::stiring from error path in castInt, but the castInt function also started failing after this commit 8041ae5 (for the same reason as castFloat); earlier there was no dependency so it worked. Since it is not a header only library, we need to either compile all the dependent cc files, along with the precompiled functions, to llvm bit code; or move this function to a stub function. I think its better to move it to a stub function since the dependencies may change.

@praveenbingo
Copy link
Contributor

also looks like this needs a rebase

Copy link
Contributor

@praveenbingo praveenbingo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

kszucs pushed a commit that referenced this pull request Oct 19, 2020
…ions

Moving the castint/float functions to gdv_function_stubs outside of precompiled module

Closes #8096 from projjal/castint and squashes the following commits:

85179a5 <Projjal Chanda> moved castInt to gdv_fn_stubs
c09077e <Projjal Chanda> fixed castfloat function
ddc429d <Projjal Chanda> added java test case
f666f54 <Projjal Chanda> fix error handling in castint

Authored-by: Projjal Chanda <[email protected]>
Signed-off-by: Praveen <[email protected]>
projjal added a commit to projjal/arrow that referenced this pull request Nov 14, 2020
…ions

Moving the castint/float functions to gdv_function_stubs outside of precompiled module

Closes apache#8096 from projjal/castint and squashes the following commits:

85179a5 <Projjal Chanda> moved castInt to gdv_fn_stubs
c09077e <Projjal Chanda> fixed castfloat function
ddc429d <Projjal Chanda> added java test case
f666f54 <Projjal Chanda> fix error handling in castint

Authored-by: Projjal Chanda <[email protected]>
Signed-off-by: Praveen <[email protected]>
pribor pushed a commit to GlobalWebIndex/arrow that referenced this pull request Oct 24, 2025
…ions

Moving the castint/float functions to gdv_function_stubs outside of precompiled module

Closes apache#8096 from projjal/castint and squashes the following commits:

85179a5 <Projjal Chanda> moved castInt to gdv_fn_stubs
c09077e <Projjal Chanda> fixed castfloat function
ddc429d <Projjal Chanda> added java test case
f666f54 <Projjal Chanda> fix error handling in castint

Authored-by: Projjal Chanda <[email protected]>
Signed-off-by: Praveen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants